[go/mysql] Avoid static buffer and use tiered pool in by LK4D4 · Pull Request #4182 · vitessio/vitess

LK4D4 · 2018-09-05T15:25:16Z

Benchmark results:

benchmark                            old ns/op     new ns/op     delta
BenchmarkParallelShortQueries-8      3259          3368          +3.34%
BenchmarkParallelMediumQueries-8     6444          6561          +1.82%
BenchmarkParallelRandomQueries-8     5963212       5441551       -8.75%

benchmark                            old MB/s     new MB/s     speedup
BenchmarkParallelShortQueries-8      3.37         3.27         0.97x
BenchmarkParallelMediumQueries-8     2543.26      2497.78      0.98x

benchmark                            old allocs     new allocs     delta
BenchmarkParallelShortQueries-8      23             23             +0.00%
BenchmarkParallelMediumQueries-8     9              8              -11.11%
BenchmarkParallelRandomQueries-8     14             14             +0.00%

benchmark                            old bytes     new bytes     delta
BenchmarkParallelShortQueries-8      720           812           +12.78%
BenchmarkParallelMediumQueries-8     27205         22720         -16.49%
BenchmarkParallelRandomQueries-8     35898388      29652823      -17.40%

I added tiered pool here because without it performance hit is too big (apparently because we use smaller buffer for readOnePacket or something and then when we read query with very high probability buffer from the pool is that smaller buffer).
cc @danieltahara

LK4D4 · 2018-09-05T15:26:10Z

go/bucketpool/bucketpool.go

I benchmarked and there is no difference between New and if i == nil way now.

there should be at least 1 alloc/op due to the interface{} framing. probably irrelevant for the simplification.

sougou

Initial comments from eyeballing.

sougou · 2018-09-05T16:01:40Z

go/bucketpool/bucketpool.go

return p.pools[len(p.pools)-1] instead.

Not sure I understand why :/ it will return last pool which contains buffers smaller than size.

Oh, even though it shouldn't be reachable...
Maybe panic? :)

sougou · 2018-09-05T16:03:45Z

go/bucketpool/bucketpool.go

Use an expression instead. It will be complex, but efficient (and worth it). This also means that maxSize is not needed.
And write lots of tests to prove it's correct, especially boundary conditions :).

I don't understand what "expression" means here :)

Ooooh, I got it. Sorry :) Will do.

LK4D4 · 2018-09-05T16:23:29Z

@sougou I actually split bucketpool change to #4183 for easier review.

sougou · 2018-09-05T16:36:51Z

I'll clarify my comments in the other PR

danieltahara

obviously also pending rebase on the other diff.

would be curious to see the delta between bucket pool and here

danieltahara · 2018-09-05T20:19:01Z

go/bucketpool/bucketpool.go

there should be at least 1 alloc/op due to the interface{} framing. probably irrelevant for the simplification.

danieltahara · 2018-09-05T20:29:15Z

go/mysql/conn.go

i wonder if it makes sense to put "finisher" functions as return values to these? That way it prevents people from forgetting to recycle. same with the write side.

danieltahara · 2018-09-05T20:29:58Z

go/mysql/conn.go

you shouldn't need this length check anymore. same with read.

and also BigBuffer can go away (or whatever the last "policy" is)

True for writes.
but for the reads code is different for large buffers - it reads packets one by one instead of ReadAll.

ah okay. feel free to leave as is

LK4D4 · 2018-09-05T23:28:14Z

So, here is benchmark results comparing to tiered pool PR:

benchmark                            old ns/op     new ns/op     delta
BenchmarkParallelShortQueries-8      3726          3771          +1.21%
BenchmarkParallelMediumQueries-8     6016          7715          +28.24%
BenchmarkParallelRandomQueries-8     5432915       5531186       +1.81%

benchmark                            old MB/s     new MB/s     speedup
BenchmarkParallelShortQueries-8      2.95         2.92         0.99x
BenchmarkParallelMediumQueries-8     2724.08      2124.35      0.78x

benchmark                            old allocs     new allocs     delta
BenchmarkParallelShortQueries-8      23             23             +0.00%
BenchmarkParallelMediumQueries-8     8              8              +0.00%
BenchmarkParallelRandomQueries-8     14             14             +0.00%

benchmark                            old bytes     new bytes     delta
BenchmarkParallelShortQueries-8      720           815           +13.19%
BenchmarkParallelMediumQueries-8     20886         23261         +11.37%
BenchmarkParallelRandomQueries-8     29717958      30225503      +1.71%

And I don't know what could cause this slowdown... Maybe I'll try to revert to readPacketDirect... Though it's not in profile top20.

sougou · 2018-09-06T01:49:33Z

What are the benchmarks compared against?

sougou · 2018-09-06T01:53:05Z

go/bucketpool/bucketpool.go

Will this work correctly if size was 1 and minSize was 1024?

Nevermind. I see you set idx to 0 next :)

LK4D4 · 2018-09-06T03:52:22Z

@sougou against #4183
it's actually quite bizarre :(
I found that diff

diff --git a/go/mysql/conn.go b/go/mysql/conn.go
index 2ac69b54a..528bb1daa 100644
--- a/go/mysql/conn.go
+++ b/go/mysql/conn.go
@@ -166,7 +166,7 @@ type Conn struct {
        // Call startEphemeralPacket(length) to get a buffer. If length
        // is smaller or equal than connBufferSize-4, this buffer will be used.
        // Otherwise memory will be allocated for it.
-       buffer []byte
+       //buffer []byte

        // Keep track of how and of the buffer we allocated for an
        // ephemeral packet on the read and write sides.
@@ -193,7 +193,7 @@ func newConn(conn net.Conn) *Conn {
                reader:   bufio.NewReaderSize(conn, connBufferSize),
                writer:   bufio.NewWriterSize(conn, connBufferSize),
                sequence: 0,
-               buffer:   make([]byte, connBufferSize),
+               //buffer:   make([]byte, connBufferSize),
        }
 }

causes performance degradation, especially for medium queries. I have no idea why and kinda losing my mind at this point :)

sougou · 2018-09-06T03:53:54Z

I think the reason why medium buffer is slower is because the size is just above the maxSize. So, it ends up with fresh allocations for every iteration.

I spent some more time digging through the code, and have a few observations. The TL;DR is that we can get rid of all read and write policies.

For read policy, MaxPacketSize is 16M. So, big buffers will never be put back into the pool because size exceeds maxSize. This means that we don't need to track policies.
We need more buckets. I think we should go up to a maxSize of 15M. Beware of benchmarks, because a random number will skew it towards the huge maxSize. You may have to exponentiate the random numbers, or use different bands.
For writes, there is a readability problem: the different code paths are not due to policy. They are actually due to the packet size. So, the code should be changed to explicitly reflect that. This means that we can get rid of write policies.
bufio.Writer has a Reset function which allows you to change the writer. This means that you can put it in a sync.Pool.
Also, the use of direct is bug prone. It should not be a flag passed into writeEphemeralPacket. Instead, there should be a separate set of functions that cleanly encapsulate the use of bufio with a defer that flushes it (and recycles if we use sync.Pool). This should probably be a separate PR.

LK4D4 · 2018-09-06T04:25:02Z

Problem is that slowness isn't caused by any changes in code apart from removing buffer allocation from Conn. Literally, if I just allocate unused 16kb []byte - everything is perfect.
I'm not sure I follow buckets problem - maxSize is MaxPacketSize in this PR(which is 16Mb).

danieltahara · 2018-09-06T04:29:00Z

Wow great observation about reader/writer. That will make things so much simpler -- do the header read for read and then get a pooled buffered reader, and write is straughtforward

sougou · 2018-09-06T04:41:20Z

I got confused (by looking at the Pool benchmarks). I see that you're now using 16MB.

The buffer thing is weird indeed. We can look at varying the size of the medium query size to see where it jumps. That could give us a clue.

I'm wondering if pprof would help for such short runs.

LK4D4 · 2018-09-06T05:27:46Z

@sougou I tried twice bigger queries with the same result :(
Btw, adding _ [connBufferSize]byte to struct "fixes" the problem. Apparently some gc tricks again.

Just use sync.Pool always Signed-off-by: Alexander Morozov <lk4d4math@gmail.com>

danieltahara

removing my block. we can deprecate big buffers in a follow up

LK4D4 commented Sep 5, 2018

View reviewed changes

sougou reviewed Sep 5, 2018

View reviewed changes

danieltahara suggested changes Sep 5, 2018

View reviewed changes

sougou reviewed Sep 6, 2018

View reviewed changes

[go/mysql] remove static buffer from Conn

11f31cf

Just use sync.Pool always Signed-off-by: Alexander Morozov <lk4d4math@gmail.com>

danieltahara approved these changes Sep 6, 2018

View reviewed changes

sougou merged commit 643873a into vitessio:master Sep 6, 2018

Conversation

LK4D4 commented Sep 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sougou left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Sep 5, 2018

Uh oh!

sougou commented Sep 5, 2018

Uh oh!

danieltahara left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sougou commented Sep 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LK4D4 commented Sep 6, 2018

Uh oh!

sougou commented Sep 6, 2018

Uh oh!

LK4D4 commented Sep 6, 2018

Uh oh!

danieltahara commented Sep 6, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sougou commented Sep 6, 2018

Uh oh!

LK4D4 commented Sep 6, 2018

Uh oh!

danieltahara left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

LK4D4 commented Sep 5, 2018 •

edited

Loading

danieltahara commented Sep 6, 2018 via email •

edited

Loading